Conversation
WalkthroughAdds devel periodic CI for sandboxed-containers-operator: updates base_images (tests-private→ci:4.22, adds tls-scanner-tool), pins build_root/releases to ocp/4.21, sets default job resources, and adds two 72h TLS-scanner periodic jobs (default and PQC). ChangesOpenShift Sandboxed Containers Operator Periodic Configuration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ci-operator/config/openshift/sandboxed-containers-operator/openshift-sandboxed-containers-operator-devel__periodics.yaml (1)
25-138: 🏗️ Heavy liftConsider extracting duplicated Pod lifecycle steps to shared step-registry entries.
The two test definitions are 99% identical—they differ only in the
PQC_CHECKenvironment variable (line 89). The entire Pod manifest (lines 42-64 vs 99-121), creation logic, wait command, and deletion logic are duplicated across both tests.This duplication increases maintenance burden: any change to the Pod spec or lifecycle commands must be applied in two places, risking inconsistency.
Consider refactoring by:
- Creating a shared step-registry entry for peer-pod creation/deletion
- Parameterizing the Pod manifest via environment variables if needed
- Passing test-specific environment variables (like
PQC_CHECK) through the step configurationThis would reduce the ~110 lines to approximately half while preserving both test variants.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ci-operator/config/openshift/sandboxed-containers-operator/openshift-sandboxed-containers-operator-devel__periodics.yaml` around lines 25 - 138, Both periodic tests (as: tls-scanner-default and as: tls-scanner-pqc) duplicate the peer-pod Pod manifest and lifecycle steps; extract those into reusable step-registry entries and parameterize them. Create shared step-registry steps (e.g., create-peer-pod and delete-peer-pod) that accept env parameters like POD_NAME, SCAN_NAMESPACE (or NAMESPACE), RUNTIMECLASS, IMAGE, and TIMEOUT, then replace the inline commands in both test definitions with references to those steps; keep test-specific env (PQC_CHECK) only in the top-level env for tls-scanner-pqc so the common steps read parameters from the passed env. Ensure the step names match the existing ref usages (ref: create-peer-pod and ref: delete-peer-pod) so workflow sandboxed-containers-operator-e2e-aws continues to work.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@ci-operator/config/openshift/sandboxed-containers-operator/openshift-sandboxed-containers-operator-devel__periodics.yaml`:
- Around line 25-138: Both periodic tests (as: tls-scanner-default and as:
tls-scanner-pqc) duplicate the peer-pod Pod manifest and lifecycle steps;
extract those into reusable step-registry entries and parameterize them. Create
shared step-registry steps (e.g., create-peer-pod and delete-peer-pod) that
accept env parameters like POD_NAME, SCAN_NAMESPACE (or NAMESPACE),
RUNTIMECLASS, IMAGE, and TIMEOUT, then replace the inline commands in both test
definitions with references to those steps; keep test-specific env (PQC_CHECK)
only in the top-level env for tls-scanner-pqc so the common steps read
parameters from the passed env. Ensure the step names match the existing ref
usages (ref: create-peer-pod and ref: delete-peer-pod) so workflow
sandboxed-containers-operator-e2e-aws continues to work.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 9353e89e-afd5-4951-89ae-1c3d364881f0
⛔ Files ignored due to path filters (1)
ci-operator/jobs/openshift/sandboxed-containers-operator/openshift-sandboxed-containers-operator-devel-periodics.yamlis excluded by!ci-operator/jobs/**
📒 Files selected for processing (1)
ci-operator/config/openshift/sandboxed-containers-operator/openshift-sandboxed-containers-operator-devel__periodics.yaml
|
/pj-rehearse periodic-ci-openshift-sandboxed-containers-operator-devel-periodics-tls-scanner-default |
|
@thejasn: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
86c7095 to
31b37be
Compare
|
/pj-rehearse periodic-ci-openshift-sandboxed-containers-operator-devel-periodics-tls-scanner-default |
|
@thejasn: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse periodic-ci-openshift-sandboxed-containers-operator-devel-periodics-tls-scanner-default |
|
@thejasn: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse periodic-ci-openshift-sandboxed-containers-operator-devel-periodics-tls-scanner-default |
|
@thejasn: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
31b37be to
5edcfdb
Compare
|
/pj-rehearse periodic-ci-openshift-sandboxed-containers-operator-devel-periodics-tls-scanner-default |
|
@thejasn: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse periodic-ci-openshift-sandboxed-containers-operator-devel-periodics-tls-scanner-default |
|
@thejasn: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse periodic-ci-openshift-sandboxed-containers-operator-devel-periodics-tls-scanner-default |
|
@thejasn: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ci-operator/config/openshift/sandboxed-containers-operator/openshift-sandboxed-containers-operator-devel__periodics.yaml (1)
88-145:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winConsider adding
PODVM_INSTANCE_TYPESfor consistency.The
tls-scanner-defaulttest explicitly setsPODVM_INSTANCE_TYPES: t3.medium(line 37), but this test omits it. Since both tests enable peer pods (ENABLEPEERPODS: "true"), explicitly setting the instance type in both tests would improve consistency and reduce the risk of divergent behavior if defaults change.📝 Proposed addition
env: AWS_REGION_OVERRIDE: us-east-2 ENABLEPEERPODS: "true" PQC_CHECK: "true" + PODVM_INSTANCE_TYPES: t3.medium RUNTIMECLASS: kata-remote SCAN_NAMESPACE: openshift-sandboxed-containers-operator🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ci-operator/config/openshift/sandboxed-containers-operator/openshift-sandboxed-containers-operator-devel__periodics.yaml` around lines 88 - 145, The tls-scanner-pqc periodic job omits PODVM_INSTANCE_TYPES while tls-scanner-default sets PODVM_INSTANCE_TYPES: t3.medium; add PODVM_INSTANCE_TYPES: t3.medium to the job's env block (near ENABLEPEERPODS, PQC_CHECK, RUNTIMECLASS, etc.) in the tls-scanner-pqc definition (as: tls-scanner-pqc) so both peer-pods tests explicitly use the same instance type.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@ci-operator/config/openshift/sandboxed-containers-operator/openshift-sandboxed-containers-operator-devel__periodics.yaml`:
- Around line 88-145: The tls-scanner-pqc periodic job omits
PODVM_INSTANCE_TYPES while tls-scanner-default sets PODVM_INSTANCE_TYPES:
t3.medium; add PODVM_INSTANCE_TYPES: t3.medium to the job's env block (near
ENABLEPEERPODS, PQC_CHECK, RUNTIMECLASS, etc.) in the tls-scanner-pqc definition
(as: tls-scanner-pqc) so both peer-pods tests explicitly use the same instance
type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 3cf72de6-0f4c-4518-9da5-8adaf33dc2a4
📒 Files selected for processing (3)
ci-operator/config/openshift/sandboxed-containers-operator/openshift-sandboxed-containers-operator-devel__periodics.yamlci-operator/step-registry/sandboxed-containers-operator/peerpods/param-cm/sandboxed-containers-operator-peerpods-param-cm-commands.shci-operator/step-registry/sandboxed-containers-operator/peerpods/param-cm/sandboxed-containers-operator-peerpods-param-cm-ref.yaml
e588601 to
f55ed6e
Compare
|
/pj-rehearse periodic-ci-openshift-sandboxed-containers-operator-devel-periodics-tls-scanner-default |
|
@thejasn: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse periodic-ci-openshift-sandboxed-containers-operator-devel-periodics-tls-scanner-pqc |
|
@thejasn: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
0fdecc6 to
c35f8f0
Compare
|
/pj-rehearse periodic-ci-openshift-sandboxed-containers-operator-devel-periodics-tls-scanner-pqc |
|
@thejasn: |
|
/pj-rehearse periodic-ci-openshift-sandboxed-containers-operator-devel-periodics-tls-scanner-pqc |
|
@thejasn: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
1 similar comment
|
@thejasn: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse periodic-ci-openshift-sandboxed-containers-operator-devel-periodics-tls-scanner-pqc |
|
@thejasn: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse periodic-ci-openshift-sandboxed-containers-operator-devel-periodics-tls-scanner-default |
|
@thejasn: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse periodic-ci-openshift-sandboxed-containers-operator-devel-periodics-tls-scanner-pqc |
|
@thejasn: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
Add two 72h periodic jobs on the devel branch to verify TLS compliance of the sandboxed-containers operator stack with an active peer pod: - tls-scanner-default: baseline TLS scan (certs, ciphers, protocols) - tls-scanner-pqc: PQC readiness check (TLS 1.3 + ML-KEM/X25519MLKEM768) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Thejas N <thn@redhat.com>
11eb891 to
6616a6e
Compare
|
/pj-rehearse periodic-ci-openshift-sandboxed-containers-operator-devel-periodics-tls-scanner-default /pj-rehearse periodic-ci-openshift-sandboxed-containers-operator-devel-periodics-tls-scanner-pqc |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: thejasn The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@thejasn: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
[REHEARSALNOTIFIER]
Interacting with pj-rehearseComment: Once you are satisfied with the results of the rehearsals, comment: |
|
@thejasn: requesting more than one rehearsal in one comment is not supported. If you would like to rehearse multiple specific jobs, please separate the job names by a space in a single command. |
|
/pj-rehearse periodic-ci-openshift-sandboxed-containers-operator-devel-periodics-tls-scanner-pqc |
|
@thejasn: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
/pj-rehearse periodic-ci-openshift-sandboxed-containers-operator-devel-periodics-tls-scanner-pqc |
|
@thejasn: now processing your pj-rehearse request. Please allow up to 10 minutes for jobs to trigger or cancel. |
|
@thejasn: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Add two 72h periodic jobs on the devel branch to verify TLS compliance of the sandboxed-containers operator stack with an active peer pod:
Updates the OpenShift CI periodic configuration for the openshift/sandboxed-containers-operator devel periodics to add two TLS-scanner periodic jobs and related CI inputs, and tweaks peerpod parameterization.
Practical impact
CI image and build inputs
Peerpod parameterization change
Resources and other notes
Related to: rhjira#KATA-5101